Mismatch for Conway predicate failures #4666

aniketd · 2024-10-07T11:37:14Z

Description

This is the second (Conway) part of #4619.

Checklist

Commit sequence broadly makes sense and commits have useful messages
New tests are added if needed and existing tests are updated
All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages.
New section is never added with the code changes. (See RELEASING.md)
When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
versioning process.
The version bounds in .cabal files for all affected packages are updated.
If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
Code is formatted with fourmolu (use scripts/fourmolize.sh)
Cabal files are formatted (use scripts/cabal-format.sh)
hie.yaml has been updated (use scripts/gen-hie.sh)
Self-reviewed the diff

neilmayhew

Very nice improvement! Your handling of the protocol encoding variations is neat, too.

My suggestions are mostly cosmetic.

However, there's a more substantial one at the end. It doesn't affect the functionality, but it does affect the readability.

I've also suggested a few additional places where Mismatch could be used.

eras/conway/impl/src/Cardano/Ledger/Conway/Rules/Ledger.hs

eras/conway/impl/src/Cardano/Ledger/Conway/Rules/Utxo.hs

eras/conway/impl/src/Cardano/Ledger/Conway/Rules/Utxow.hs

libs/cardano-ledger-test/src/Test/Cardano/Ledger/Generic/PrettyCore.hs

eras/conway/impl/src/Cardano/Ledger/Conway/Rules/Bbody.hs

lehins

All my comments in BBODY rule module apply to all other rules affected by this PR.

eras/conway/impl/src/Cardano/Ledger/Conway/Rules/Bbody.hs

eras/conway/impl/src/Cardano/Ledger/Conway/Rules/Utxow.hs

TimSheard

OK I only commented on the Coders parts.
The extra constraint should not be there. We should try hrd to get rid of it.

libs/cardano-ledger-binary/src/Cardano/Ledger/Binary/Decoding/Coders.hs

lehins

This is looking great!
A few more simplifications and improvements and it will ready to go

lehins · 2024-10-17T18:17:55Z

libs/cardano-ledger-core/src/Cardano/Ledger/BaseTypes.hs

+swapMismatch :: Mismatch r a -> Mismatch r a
+swapMismatch m@(Mismatch _ _) =
+  let Mismatch {..} = m
+   in Mismatch
+        { mismatchSupplied = mismatchExpected
+        , mismatchExpected = mismatchSupplied
+        }


This is just a terrible hack. Why would you do something like this: mismatchSupplied = mismatchExpected?

If you have a type that has some invariant or just a semantic meaning, you should try to preserve it at all times, regardless of the fact that its use could be ephemeral. In other words if something is "expected" it can't all of a sudden become "supplied"

Just use a tuple instead of this hack. Also we can never add any new fields to Mismatch, so there is no need to use this trick with pattern matching

Suggested change

swapMismatch :: Mismatch r a -> Mismatch r a

swapMismatch m@(Mismatch _ _) =

let Mismatch {..} = m

in Mismatch

{ mismatchSupplied = mismatchExpected

, mismatchExpected = mismatchSupplied

}

-- | Convert a `Mismatch` to a tuple that has "supplied" and "expected" swapped places

swapMismatch :: Mismatch r a -> (a, a)

swapMismatch Mismatch {mismatchSupplied, mismatchExpected} =

(mismatchExpected, mismatchSupplied)

-- | Convert a tuple that has "supplied" and "expected" swapped places to a `Mismatch` type.

unswapMismatch :: (a, a) -> Mismatch r a

unswapMismatch (mismatchExpected, mismatchSupplied) =

Mismatch {mismatchSupplied, mismatchExpected}

Thank you! This makes it easier to understand what's going on.

I agree 💯

I tried this with specifying {Enc,Dec}CBORGroup instance for (a, a) and the types end up disagreeing because the Coders require the element to be of type Mismatch. Let me know if you have an idea around it. <$> and MapE also won't work.

libs/cardano-ledger-core/src/Cardano/Ledger/BaseTypes.hs

libs/cardano-ledger-binary/src/Cardano/Ledger/Binary/Decoding/Coders.hs

libs/cardano-ledger-test/src/Test/Cardano/Ledger/Generic/PrettyCore.hs

libs/cardano-ledger-binary/src/Cardano/Ledger/Binary/Encoding/Coders.hs

eras/conway/impl/src/Cardano/Ledger/Conway/Rules/Utxow.hs

libs/cardano-ledger-binary/cardano-ledger-binary.cabal

Also, add {Enc,Dec}CBORGroup instances for Mismatch

Rules: - BBODY - GOV - GOVCERT - LEDGER - UTXO - UTXOW

aniketd changed the title ~~Aniketd/mismatch for conway~~ Mismatch for Conway predicate failures Oct 7, 2024

aniketd force-pushed the aniketd/mismatch-type-for-predicate-failures branch from 87dba8f to a6572dd Compare October 8, 2024 10:13

Base automatically changed from aniketd/mismatch-type-for-predicate-failures to master October 8, 2024 12:26

aniketd force-pushed the aniketd/mismatch-for-conway branch 3 times, most recently from 7e89bac to 99cee0e Compare October 10, 2024 19:53

aniketd marked this pull request as ready for review October 10, 2024 19:53

aniketd requested a review from a team as a code owner October 10, 2024 19:53

aniketd requested review from teodanciu and lehins October 10, 2024 19:53

neilmayhew requested changes Oct 10, 2024

View reviewed changes

teodanciu reviewed Oct 11, 2024

View reviewed changes

eras/conway/impl/src/Cardano/Ledger/Conway/Rules/Bbody.hs Outdated Show resolved Hide resolved

aniketd force-pushed the aniketd/mismatch-for-conway branch from 99cee0e to 0ff6cfb Compare October 14, 2024 14:49

aniketd requested review from neilmayhew and teodanciu October 14, 2024 14:56

lehins requested changes Oct 14, 2024

View reviewed changes

lehins mentioned this pull request Oct 15, 2024

Switch all relevant PredicateFailures to use NonEmpty #4066

Open

aniketd force-pushed the aniketd/mismatch-for-conway branch 2 times, most recently from 0437261 to 3f8dd45 Compare October 17, 2024 15:22

aniketd requested review from lehins and TimSheard October 17, 2024 15:28

aniketd force-pushed the aniketd/mismatch-for-conway branch from 3f8dd45 to c47d53d Compare October 17, 2024 15:37

TimSheard reviewed Oct 17, 2024

View reviewed changes

libs/cardano-ledger-binary/src/Cardano/Ledger/Binary/Decoding/Coders.hs Outdated Show resolved Hide resolved

lehins requested changes Oct 17, 2024

View reviewed changes

aniketd force-pushed the aniketd/mismatch-for-conway branch 3 times, most recently from 34b1182 to e8ec767 Compare October 18, 2024 12:05

aniketd requested review from TimSheard and lehins October 18, 2024 12:07

teodanciu reviewed Oct 18, 2024

View reviewed changes

libs/cardano-ledger-binary/cardano-ledger-binary.cabal Show resolved Hide resolved

aniketd added 3 commits October 19, 2024 19:21

Add {To,From}Group to Coders for {Enc,Dec}CBORGroup

2b46e66

Also, add {Enc,Dec}CBORGroup instances for Mismatch

Use Mismatch for Conway pred-failures.

0d14fa2

Rules: - BBODY - GOV - GOVCERT - LEDGER - UTXO - UTXOW

Bump versions and add changelog

1e058b9

aniketd force-pushed the aniketd/mismatch-for-conway branch from 44395aa to 1e058b9 Compare October 19, 2024 13:51

aniketd requested a review from teodanciu October 19, 2024 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mismatch for Conway predicate failures #4666

Mismatch for Conway predicate failures #4666

aniketd commented Oct 7, 2024 •

edited

Loading

neilmayhew left a comment •

edited

Loading

lehins left a comment

TimSheard left a comment

lehins left a comment

lehins Oct 17, 2024

aniketd Oct 18, 2024

aniketd Oct 18, 2024

aniketd Oct 18, 2024

-swapMismatch :: Mismatch r a -> Mismatch r a
-swapMismatch m@(Mismatch _ _) =
-  let Mismatch {..} = m
-   in Mismatch
-        { mismatchSupplied = mismatchExpected
-        , mismatchExpected = mismatchSupplied
-        }
+-- | Convert a `Mismatch` to a tuple that has "supplied" and "expected" swapped places
+swapMismatch :: Mismatch r a -> (a, a)
+swapMismatch Mismatch {mismatchSupplied, mismatchExpected} =
+  (mismatchExpected, mismatchSupplied)
+-- | Convert a tuple that has "supplied" and "expected" swapped places to a `Mismatch` type.
+unswapMismatch :: (a, a) -> Mismatch r a
+unswapMismatch (mismatchExpected, mismatchSupplied) =
+  Mismatch {mismatchSupplied, mismatchExpected}

Mismatch for Conway predicate failures #4666

Are you sure you want to change the base?

Mismatch for Conway predicate failures #4666

Conversation

aniketd commented Oct 7, 2024 • edited Loading

Description

Checklist

neilmayhew left a comment • edited Loading

Choose a reason for hiding this comment

lehins left a comment

Choose a reason for hiding this comment

TimSheard left a comment

Choose a reason for hiding this comment

lehins left a comment

Choose a reason for hiding this comment

lehins Oct 17, 2024

Choose a reason for hiding this comment

aniketd Oct 18, 2024

Choose a reason for hiding this comment

aniketd Oct 18, 2024

Choose a reason for hiding this comment

aniketd Oct 18, 2024

Choose a reason for hiding this comment

aniketd commented Oct 7, 2024 •

edited

Loading

neilmayhew left a comment •

edited

Loading